docs: add ADR for defining API for querying permissions from MFEs for current user FC-0099#60
Conversation
|
Thanks for the pull request, @rodmgwgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
3ef15d2 to
24192a5
Compare
|
The proposed mechanism sounds fine to me, but I wonder if we should also include a standardized way to include permissions data in "regular" backend API responses. For example, consider an MFE that is getting a list of libraries, and the user has "edit" permission on some of the libraries but not others. The MFE would first have to GET Because these queries cannot happen simultaneously1, this means that either there is a delay in showing the edit buttons (so the users see the list of libraries after it's loaded, but none of them show the "Edit" button until the permissions have also loaded, and then there is a flash of change as the edit buttons get rendered once we hear back from the authz API), or we can choose to wait until both things have loaded before showing the list of libraries, which makes the MFE feel slower. To solve this in the Authoring MFE today (and many other parts of the platform), we use a very simple solution - include the permissions data with the "list objects" API results (example). So, I think that this ADR should also include a standard recommendation for including a "permissions" object attached to the results of most regular Footnotes
|
Thanks for this insight, definitely having the permissions information in regular API responses would simplify things going forward. In the case of libraries, we can keep the existing information in the list objects endpoint and adapt the underlying logic to use the new authz library for evaluation. I'm not sure how we can generalize this so we have a single standard way of including these permissions information for any kind of resource. Perhaps, going forward we could standardize this by including in the object a property like: Where key is the "action", and the boolean indicates if it's allowed or not. What do you think? |
|
I like the idea of something like that, yeah. Interested to hear what other people think. |
| required format. | ||
| - 401: Unauthorized, happens when the user is not authenticated/logged in. | ||
|
|
||
| **Please note:** There is no “404 not found” case here, if the action, |
There was a problem hiding this comment.
I just have a question here, I'm not an expert but in my understanding Casbin will evaluate the query as denied and return false, in this kind of scenarios, if so will be difficult to the frontend give an appropriate feedback to the user whether does not have permission or the resource does not exist. Do we have this contemplated?
As much as I understand, based on a small searching and that is explained here https://ridouku.medium.com/casbin-an-alternative-to-validate-user-permissions-fd21fadc2b3a it is needed to wrap the enforcer in a function to make that validations and return the custom error.
There was a problem hiding this comment.
Yes, that's my understanding, Casbin will just validate it as not allowed as it doesn't have knowledge of the actual resources on the database.
If we wanted to validate if, for example, a library exists, we would have to identify that the object we are targeting is a library (by parsing the lib: prefix), and then query the DB. This would add extra DB queries and logic which would impact on performance and complexity for maintainability.
I'm not sure if we really need to differentiate between a permission query on a non-existing resource vs just a false response. Do you see a use case where this would be important? The main use cases I have thought of are related to things like disabling edit buttons and hiding "view details" links.
There was a problem hiding this comment.
I'm with @rodmgwgu on this, I think this module should be focused on whether or not the permission check passes. I can't think of a case where the front end would be querying for permissions of something that doesn't exist aside from the case of a race condition deleting a resource. Even in that case I don't think it should be authz's responsibility to communicate that information.
24192a5 to
bbb69ea
Compare
bbb69ea to
2f52a4d
Compare
bmtcril
left a comment
There was a problem hiding this comment.
This looks good to me, thanks for writing it up!
| [ | ||
| { | ||
| "action": "act:read", | ||
| "object": "lib:test-lib", | ||
| "scope": "org:OpenedX" | ||
| }, | ||
| { | ||
| "action": "act:edit", | ||
| "object": "lib:test-lib", | ||
| "scope": "org:OpenedX" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Does this mean that if a user has permission on a scope, they automatically have permission on the object within it? If so, our decision is that scope inheritance will not be managed implicitly - sorry it wasn't part of an ADR, now it is: #76. Instead, checks must explicitly handle containment.
For example:
can(user, act:read, lib:test-lib) OR can(user, act:read, org:OpenedX)
Here, lib:test-lib and org:OpenedX are treated as separate scopes. I’m not sure if this belongs directly in this ADR, but it’s worth noting for clarity. Still I think this ADR covers this case as well, since object is optional.
FYI @bmtcril
There was a problem hiding this comment.
I agree that the distinction is important and should probably be explicitly covered here as well.
There was a problem hiding this comment.
@mariajgrimaldi does this mean that there won't be any cases where we want to specify both scope and object when checking for permissions with casbin? will scope and object will be the same thing now for casbin going forward?
What I'm understanding is that we will no longer have an "object" here, it will always go into "scope", even if it's an org, lib, or something else:
[
{
"action": "act:read",
"scope": "lib:test-lib"
},
{
"action": "act:edit",
"scope": "org:OpenedX"
}
]
Is this correct?
There was a problem hiding this comment.
I updated the doc to reflect these changes and match what is being implemented on #84
There was a problem hiding this comment.
@rodmgwgu: that's the level of granularity we're going with for now. It's not set in stone, since the model could later evolve to be more granular. For now, let's note that possibility but focus on describing the current, more feasible approach.
There was a problem hiding this comment.
@mariajgrimaldi is this a request for a change or can this merge?
There was a problem hiding this comment.
Merged! Sorry I didn't see this earlier :)
… from MFEs and other remote clients
mariajgrimaldi
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the patience :)
ADR for defining an enforcement mechanism for MFEs and other clients, here we discuss about effective ways to achieve this and propose an API endpoint for permission querying.
Merge checklist:
Check off if complete or not applicable: